Add currency utilities and helpers.#491
Add currency utilities and helpers.#491akx merged 1 commit intopython-babel:masterfrom kdeldycke:currency-utils
Conversation
gitmate-bot
added
pending review
process/pending review
size/M
and removed
pending review
labels
Codecov Report
@@ Coverage Diff @@
## master #491 +/- ##
==========================================
+ Coverage 90.19% 90.25% +0.06%
==========================================
Files 24 24
Lines 3977 4004 +27
==========================================
+ Hits 3587 3614 +27
Misses 390 390
Continue to review full report at Codecov.
|
babel/numbers.py
Outdated
|
|
||
|
|
||
| def get_currency_name(currency, count=None, locale=LC_NUMERIC): | ||
| def list_currency(locale=None): |
There was a problem hiding this comment.
This should be list_currencies, I think.
There was a problem hiding this comment.
Done in 9b23174. Thanks for the feedback! :)
babel/numbers.py
Outdated
| Locale.parse(l).currencies.keys() for l in locale_identifiers()])) | ||
|
|
||
|
|
||
| def is_currency(currency_code, locale=None): |
There was a problem hiding this comment.
Maybe is_currency_supported? Or validate_currency?
There was a problem hiding this comment.
The validation of currency is really what I'm trying to do here.
Now I'm used to have an is_currency and validate_currency couple helpers when it comes to data validation. I modified the code in f89d4d2 to reflect that. Do you like it?
babel/numbers.py
Outdated
| provided, returns the list of all currencies from all | ||
| locales. | ||
| """ | ||
| # Get locale-scopped currencies. |
babel/numbers.py
Outdated
|
|
||
| def get_currency_name(currency, count=None, locale=LC_NUMERIC): | ||
| def list_currency(locale=None): | ||
| """ Return a list of normalized currency codes. |
There was a problem hiding this comment.
The docstring is lying in that this may return either a dict_keys object (when locale is passed in) or a set, but never a list.
Maybe always return a set?
There was a problem hiding this comment.
Oh yes. Makes total sense. Done in 7ce48ee.
babel/numbers.py
Outdated
| return Locale.parse(locale).currencies.keys() | ||
| # Aggregate currency codes from all locales. | ||
| return set(chain.from_iterable([ | ||
| Locale.parse(l).currencies.keys() for l in locale_identifiers()])) |
There was a problem hiding this comment.
Yigh, parsing and instantiating every known locale sounds a little expensive, especially since most of this will probably end up being a no-op (in that there's likely no specific language that provides a currency code the other languages don't know about).
Could this be done in some other way? Maybe at CLDR import time? An all_currencies key in the global data? Could currency_fractions be used?
There was a problem hiding this comment.
I'm not too happy with that brute force approach either. At the time I wrote that I wasn't really certain about the completeness of the CLDR data, hence the way I took.
I think I'll first ensure with a unit-test that, as you said, there's no specific language providing a currency code any other languages don't know about. If we can verify this assumption then I'll try to dig into the CLDR import code and see what I can do.
There was a problem hiding this comment.
So that was what I suspected all along. The reason I'm iterating on all locales is that not all locales have the same knowledge of currencies:
>>> from babel.core import Locale
>>> from babel.localedata import locale_identifiers
>>> import babel
>>> babel.__version__
'2.4.0'
>>> currency_sets = set()
>>> for locale in locale_identifiers():
... currency_sets.add(frozenset(Locale.parse(locale).currencies.keys()))
...
>>> currency_sets
set([frozenset([]),
frozenset(['CVE', 'NGN']),
frozenset(['XAF']),
frozenset(['AZN']),
(...)
frozenset(['AED',
'AFA',
'AFN',
'ALL',
'AMD',
'ANG',
'AOA',
'ARS',
'AUD',
'AWG',
'AZN',
'BAM',
'BBD',
'BDT',
'BGN',
'BHD',
'BIF',
'BMD',
'BND',
'BOB',
'BRL',
'BSD',
'BTN',
'BWP',
'BYR',
'BZD',
'CAD',
'CDF',
'CHF',
'CLP',
'CNY',
'COP',
'CRC',
'CUC',
'CUP',
'CVE',
'CZK',
'DJF',
'DKK',
'DOP',
'DZD',
'EGP',
'ERN',
'ETB',
'EUR',
'FJD',
'FKP',
'GBP',
'GEL',
'GHS',
'GIP',
'GMD',
'GNF',
'GTQ',
'GYD',
'HKD',
'HNL',
'HRK',
'HTG',
'HUF',
'IDR',
'ILS',
'INR',
'IQD',
'IRR',
'ISK',
'JMD',
'JOD',
'JPY',
'KES',
'KGS',
'KHR',
'KMF',
'KPW',
'KRW',
'KWD',
'KYD',
'KZT',
'LAK',
'LBP',
'LKR',
'LRD',
'LTL',
'LVL',
'LYD',
'MAD',
'MDL',
'MGA',
'MKD',
'MMK',
'MNT',
'MOP',
'MRO',
'MUR',
'MVR',
'MWK',
'MXN',
'MYR',
'MZN',
'NAD',
'NGN',
'NIO',
'NOK',
'NPR',
'NZD',
'OMR',
'PAB',
'PEN',
'PGK',
'PHP',
'PKR',
'PLN',
'PYG',
'QAR',
'RON',
'RSD',
'RUB',
'RWF',
'SAR',
'SBD',
'SCR',
'SDG',
'SEK',
'SGD',
'SHP',
'SLL',
'SOS',
'SRD',
'SSP',
'STD',
'SYP',
'SZL',
'THB',
'TJS',
'TMT',
'TND',
'TOP',
'TRY',
'TTD',
'TWD',
'TZS',
'UAH',
'UGX',
'USD',
'UYU',
'UZS',
'VEF',
'VND',
'VUV',
'WST',
'XAF',
'XCD',
'XOF',
'XPF',
'XXX',
'YER',
'ZAR',
'ZMK',
'ZWR'])])
>>> len(currency_sets)
105There was a problem hiding this comment.
Progress note: currency_fractions is partial. So I'll go the all_currencies route.
babel/numbers.py
Outdated
| try: | ||
| if currency_code not in list_currency(locale): | ||
| return False | ||
| except (ValueError, UnknownLocaleError): |
There was a problem hiding this comment.
Hmmm... Maybe UnknownLocaleError shouldn't be swallowed? Also, when might one expect a ValueError?
babel/numbers.py
Outdated
|
|
||
|
|
||
| def normalize_currency(currency_code, locale=None): | ||
| """ Normalize any currency code string. |
There was a problem hiding this comment.
Hmm, so "normalization" here means just upper-casing the code, then validating it?
The docstring should at least let the user know that ValueError will be raised if the code is not known to Babel.
There was a problem hiding this comment.
Currently yes: normalization is only upper-casing. But that method allows us to enhance normalization in more clever way in the future, if the need arises.
As for the ValueError, I just did the change in 5d9f416.
babel/numbers.py
Outdated
| .. versionadded:: 0.9.4 | ||
|
|
||
| :param currency: the currency code | ||
| :param currency: the currency ISO code. |
There was a problem hiding this comment.
If you're changing the names of the parameters, also change the docstrings to match. That said, I'm not sure currency needs to be changed to currency_code throughout -- everyone should know it's a code. (Not to mention changing these parameter names would also be a semver-major change.)
There was a problem hiding this comment.
Oh yes, sorry for that. It was too ambitious and currency as it is now is good enough.
I reverted in commit 5ba7c7b.
|
|
||
|
|
||
| def currency_precision(currency_code): | ||
| """Return currency's precision. |
There was a problem hiding this comment.
A description of what "precision" means would be nice.
babel/numbers.py
Outdated
| currency_code, currency_code) | ||
|
|
||
|
|
||
| def currency_precision(currency_code): |
There was a problem hiding this comment.
This oughta be get_currency_precision (to have a verb in all method names).
|
Thanks @akx for the thorough and quick review! I'm going to answer and fix each of your comment monday. |
|
@kdeldycke Sure thing! Enjoy your weekend! |
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
5 similar comments
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
akx
left a comment
akx
left a comment
There was a problem hiding this comment.
Looking good except for the couple minor comments. Also, can you squash the fix commits into the original commits, and add tests?
babel/numbers.py
Outdated
| Raises a `ValueError` exception if the currency is unknown to Babel. | ||
| """ | ||
| if currency not in list_currencies(locale): | ||
| raise ValueError("Unknown currency {!r}.".format(currency)) |
There was a problem hiding this comment.
Maybe a custom UnknownCurrencyError exception (that derives from ValueError) could be useful here? Easier to catch in caller functions, too :)
babel/numbers.py
Outdated
| """ | ||
| return Locale.parse(locale).currency_symbols.get(currency, currency) | ||
| return Locale.parse(locale).currency_symbols.get( | ||
| currency, currency) |
There was a problem hiding this comment.
Super minor nitpick, but no need to re-wrap this line :)
We have max-line-length 120 in the project.
|
Thanks @akx for that second round of reviews. Going to fix all remaining issues and add tests. I'll notify you once everything is clean and tidy. |
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
1 similar comment
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
4 similar comments
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
|
Comment on 6fd2539, file babel/numbers.py, line 25. This file contains unused source code. PyUnusedCodeBear, severity NORMAL, section The issue can be fixed by applying the following patch: --- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -22,7 +22,10 @@
from datetime import date as date_, datetime as datetime_
from itertools import chain
-from babel.core import default_locale, Locale, get_global, UnknownLocaleError
+from babel.core import Locale
+from babel.core import UnknownLocaleError
+from babel.core import default_locale
+from babel.core import get_global
from babel._compat import decimal
from babel.localedata import locale_identifiers
|
|
Just added unit-tests on all methods and squashed commits. The only thing left to do is to get rid of the brute force approach in |
|
I'm finished working on this PR. All issues have been addressed. @akx : unless you have further comments to add, this PR is ready to be merged. |
|
The tests seems to fail for an unrelated reason... 😢 |
|
Everything's fine now. Just waiting for @akx final review before merging. 😃 |
|
So, should we merge this ? |
|
Oh yes please! Let's merge this! 😃 AFAIK this PR is just waiting for @akx to lift the reviewing embargo. |
|
Sorry for the delay! Let me just re-read the final version :) |
akx
left a comment
akx
left a comment
There was a problem hiding this comment.
Sorry! Still some slight changes required to the importer code.
scripts/import_cldr.py
Outdated
| territory_currencies[region_code] = region_currencies | ||
|
|
||
| # Deduplicate regions and makes them pickable. | ||
| all_currencies = dict([(code, tuple(set(regions))) for code, regions in all_currencies.items()]) |
There was a problem hiding this comment.
Um, this doesn't actually affect the all_currencies object that is already in global_data.
What do you mean with "pickable" (i.e. what is the requirement for the value to be a tuple)?
I mean, if the value was a set all the time, the deduplication code would be unnecessary:
for currency in region.findall('./currency'):
all_currencies.setdefault(cur_code, set()).add(region_code)There was a problem hiding this comment.
I had to resort to that unsexy code to make the metadata serialization happy. Changing the code as you proposed ends up with that failure:
____ test_compatible_classes_in_global_and_localedata[babel/global.dat] ____
filename = 'babel/global.dat'
@pytest.mark.parametrize('filename', [
'babel/global.dat',
'babel/locale-data/root.dat',
'babel/locale-data/en.dat',
'babel/locale-data/en_US.dat',
'babel/locale-data/en_US_POSIX.dat',
'babel/locale-data/zh_Hans_CN.dat',
'babel/locale-data/zh_Hant_TW.dat',
'babel/locale-data/es_419.dat',
])
def test_compatible_classes_in_global_and_localedata(filename):
# Use pickle module rather than cPickle since cPickle.Unpickler is a method
# on Python 2
import pickle
class Unpickler(pickle.Unpickler):
def find_class(self, module, name):
# *.dat files must have compatible classes between Python 2 and 3
if module.split('.')[0] == 'babel':
return pickle.Unpickler.find_class(self, module, name)
raise pickle.UnpicklingError("global '%s.%s' is forbidden" %
(module, name))
with open(filename, 'rb') as f:
> return Unpickler(f).load()
tests/test_core.py:318:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/pickle.py:858: in load
dispatch[key](self)
/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/pickle.py:1090: in load_global
klass = self.find_class(module, name)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <tests.test_core.Unpickler instance at 0x10e2e9128>, module = '__builtin__', name = 'set'
def find_class(self, module, name):
# *.dat files must have compatible classes between Python 2 and 3
if module.split('.')[0] == 'babel':
return pickle.Unpickler.find_class(self, module, name)
raise pickle.UnpicklingError("global '%s.%s' is forbidden" %
> (module, name))
E UnpicklingError: global '__builtin__.set' is forbidden
tests/test_core.py:315: UnpicklingErrorThere was a problem hiding this comment.
To be honest, it looks like that test is broken, then.
__builtin__.set should be available alright on both Python 2 and Python 3.
(I think what's happening is that there is no builtin operation in the pickle format for sets, so it resorts to "let's construct a class instance of __builtin__.set" instead.)
Anyway, fair! How about
all_currencies = collections.defaultdict(set)
for currency in region.findall('./currency'):
# ...
all_currencies[cur_code].add(region_code)
# ...
global_data['all_currencies'] = {
currency: tuple(sorted(regions))
for (currency, regions)
in all_currencies.items()
}?
|
Just addressed the last comment and squashed all commits. This PR is ready to be merged. |
|
Good to go! Thanks for the code and for putting up with me ;), @kdeldycke! |
|
Thanks @akx for the merge! No worries, I'm happy to share the burden of open-source project maintenance! :) |
5 participants
Add a couple of new utilities for currencies:
list_currency: to list the whole set of currency codes supported by Babel.validate_currency.is_currency: to quickly validate if a currency is supported by Babel or not, not unlike what is currently available for locale (see:babel.localedata.exists()).normalize_currency: to normalize a currency string to the format expected by Babel.get_currency_precision: to get the natural precision of a currency.